Conversation
📝 WalkthroughWalkthroughA new Vitest test suite was added for the flags utility module, comprehensively testing environment-based flag detection logic across CI detection, runtime environment identification, color support, Node.js versioning, and platform detection with isolated test cases using dynamic imports and environment variable stubbing. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/flags.test.ts (1)
149-167:isMinimalstill misses the!hasTTYpath.
src/flags.tscomputesisMinimalfrom four operands, but this block only exercises three of them. Adding onestdout.isTTY = falsecase, plus an all-false case, would close the remaining gap and catch regressions in the final branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/flags.test.ts` around lines 149 - 167, Add tests to cover the missing branches for isMinimal: add a test that sets process.stdout.isTTY = false (mocking/restoring it around the test) and asserts importFlags().isMinimal === true, and add an additional test where none of MINIMAL, CI, NODE_ENV are set and stdout.isTTY is true (or reset to default) and assert importFlags().isMinimal === false; reference the importFlags function and the isMinimal export and ensure the stdout.isTTY manipulation is done only inside the test to avoid leaking state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/flags.test.ts`:
- Around line 3-24: The test only clears a subset of env keys so detectProvider
(in src/providers.ts) can still see other provider-related variables and flip
behavior for isMinimal/isColorSupported; update the test setup in
test/flags.test.ts to either 1) neutralize every provider env name exported/used
by detectProvider by importing the provider key list from src/providers.ts and
clearing them from process.env before each test, or 2) replace process.env
entirely for the suite with an isolated copy (e.g., save original = process.env,
set process.env = {} or a controlled object, then restore original after tests)
so detectProvider, isMinimal and isColorSupported see a deterministic
environment. Ensure you reference detectProvider and the provider keys from
src/providers.ts so new provider env names are automatically handled.
---
Nitpick comments:
In `@test/flags.test.ts`:
- Around line 149-167: Add tests to cover the missing branches for isMinimal:
add a test that sets process.stdout.isTTY = false (mocking/restoring it around
the test) and asserts importFlags().isMinimal === true, and add an additional
test where none of MINIMAL, CI, NODE_ENV are set and stdout.isTTY is true (or
reset to default) and assert importFlags().isMinimal === false; reference the
importFlags function and the isMinimal export and ensure the stdout.isTTY
manipulation is done only inside the test to avoid leaking state.
| const flagEnvKeys = [ | ||
| "CI", | ||
| "NODE_ENV", | ||
| "MODE", | ||
| "TEST", | ||
| "DEBUG", | ||
| "MINIMAL", | ||
| "NO_COLOR", | ||
| "FORCE_COLOR", | ||
| "TERM", | ||
| "GITHUB_ACTIONS", | ||
| "VERCEL", | ||
| "VERCEL_ENV", | ||
| "NOW_BUILDER", | ||
| "GITLAB_CI", | ||
| "TRAVIS", | ||
| "CIRCLECI", | ||
| "BUILDKITE", | ||
| "APPVEYOR", | ||
| "CODESANDBOX_SSE", | ||
| "CODESANDBOX_HOST", | ||
| ]; |
There was a problem hiding this comment.
Isolate the full provider env surface in test setup.
detectProvider() scans many more env names than the subset cleared here. If these tests run on an unlisted CI provider, the leaked env will make the "no CI" cases fail and can also flip isMinimal / isColorSupported. Please either neutralize every provider key from src/providers.ts or replace process.env with an isolated test env for this suite.
Also applies to: 32-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/flags.test.ts` around lines 3 - 24, The test only clears a subset of env
keys so detectProvider (in src/providers.ts) can still see other
provider-related variables and flip behavior for isMinimal/isColorSupported;
update the test setup in test/flags.test.ts to either 1) neutralize every
provider env name exported/used by detectProvider by importing the provider key
list from src/providers.ts and clearing them from process.env before each test,
or 2) replace process.env entirely for the suite with an isolated copy (e.g.,
save original = process.env, set process.env = {} or a controlled object, then
restore original after tests) so detectProvider, isMinimal and isColorSupported
see a deterministic environment. Ensure you reference detectProvider and the
provider keys from src/providers.ts so new provider env names are automatically
handled.
1 participant
Adds test coverage for
flags.ts, which had no dedicated tests despite containing the most complex boolean logic in the library (isCI,isColorSupported,isMinimal).29 tests covering every exported flag. The interesting ones are the
isCIinteraction tests - verifying thatproviderInfo.ci !== falseworks correctly (e.g., Vercel withci: falsekeepsisCIfalse while GitHub Actions without explicitcimetadata makes it true). Also catches theNO_COLORvsFORCE_COLORprecedence andMODEas a fallback forNODE_ENV.Uses
vi.resetModules()+ dynamic import to re-evaluate the module-level constants with fresh env state per test, same pattern asagents.test.tsbut adapted for constants instead of adetect*()function.Partial progress on #62.
Summary by CodeRabbit